Skip to content

refactor: Replace JSON.parse with parseJsonWithSchema (@dev-mohit06) #6207

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom
Merged
Miodec merged 9 commits into from
Feb 3, 2025
Merged

refactor: Replace JSON.parse with parseJsonWithSchema (@dev-mohit06) #6207

Miodec merged 9 commits into from
Feb 3, 2025

Conversation

@mohitpaddhariya
Copy link

@mohitpaddhariya mohitpaddhariya commented Jan 23, 2025 •
edited
Loading

Description

Replaces raw JSON parsing with schema-based validation across frontend TypeScript files to improve type safety and error handling.

Scope of Changes

  • Updated JSON parsing in:

    • account.ts
    • import-export-settings.ts
    • analytics-controller.ts
    • local-storage-with-schema.ts
    • url-handler.ts
    • commandline/lists.ts
    • test/wikipedia.ts

  • Added schema in test/custom-text.ts:

    export const customTextDataSchema = z.object({
  text: z.array(z.string()),
  mode: CustomTextModeSchema,
  limit: z.object({ 
    value: z.number(), 
    mode: CustomTextLimitModeSchema 
  }),
  pipeDelimiter: z.boolean(),
});

Benefits

  • Enhanced runtime type safety
  • More robust error handling
  • Consistent JSON parsing approach

Checks

  • Follows Conventional Commits
  • Includes GitHub username
  • Adding quotes? (N/A)
  • Adding language/theme? (N/A)

@monkeytypegeorge monkeytypegeorge added the frontend User interface or web stuff label Jan 23, 2025
fehmer
fehmer reviewed Jan 23, 2025
View reviewed changes
fehmer
fehmer reviewed Jan 23, 2025
View reviewed changes
fehmer
fehmer reviewed Jan 23, 2025
View reviewed changes
fehmer
fehmer reviewed Jan 23, 2025
View reviewed changes
fehmer
fehmer reviewed Jan 23, 2025
View reviewed changes
fehmer
fehmer reviewed Jan 23, 2025
View reviewed changes
fehmer
fehmer reviewed Jan 23, 2025
View reviewed changes
fehmer
fehmer reviewed Jan 23, 2025
View reviewed changes
fehmer
fehmer reviewed Jan 23, 2025
View reviewed changes
fehmer
fehmer reviewed Jan 23, 2025
View reviewed changes
fehmer
fehmer requested changes Jan 23, 2025
View reviewed changes
Copy link
Member

@fehmer fehmer left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hi @dev-mohit06, nice work. I added some comments for you.

@github-actions github-actions bot added the waiting for update Pull requests or issues that require changes/comments before continuing label Jan 23, 2025
fehmer
fehmer reviewed Jan 23, 2025
View reviewed changes
@github-actions github-actions bot removed the waiting for update Pull requests or issues that require changes/comments before continuing label Jan 23, 2025
Mohit Paddhariya added 2 commits January 24, 2025 03:20
Merge branch 'master' into fix/json-parse-schema
76dc5f3
refactor: update schema definitions and replace JSON.parse with parse…
475dac5 
…WithSchema (@dev-mohit06)

- Updated schema definitions for better clarity and reusability.
- Replaced  with  where applicable.
- Addressed all review comments for better code consistency.
@mohitpaddhariya
Copy link
Author

mohitpaddhariya commented Jan 24, 2025

Hey @fehmer, could you take a look at the changes?

fehmer
fehmer requested changes Jan 24, 2025
View reviewed changes
@github-actions github-actions bot added waiting for update Pull requests or issues that require changes/comments before continuing and removed waiting for update Pull requests or issues that require changes/comments before continuing labels Jan 24, 2025
@mohitpaddhariya
Copy link
Author

mohitpaddhariya commented Jan 24, 2025

Hey @fehmer, could you take a look at the changes?

@github-actions github-actions bot added the waiting for review Pull requests that require a review before continuing label Jan 24, 2025
fehmer
fehmer previously approved these changes Jan 24, 2025
View reviewed changes
Copy link
Member

@fehmer fehmer left a comment •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hi @dev-mohit06 , changes look good. Please remove the code comment on the unresolved PR comment or replace with the suggested comments.

Thank you for your contribution. Do you want to take the backend part next?

@github-actions github-actions bot removed the waiting for review Pull requests that require a review before continuing label Jan 24, 2025
@mohitpaddhariya
Copy link
Author

mohitpaddhariya commented Jan 25, 2025

Hi @fehmer,

Thank you for the feedback! I’ll address the code comment as suggested. Regarding the backend part, I’d be happy to take it up. Could you please share the details or scope of the task?

@github-actions github-actions bot added the waiting for review Pull requests that require a review before continuing label Jan 25, 2025
fehmer
fehmer previously approved these changes Jan 25, 2025
View reviewed changes
@github-actions github-actions bot removed the waiting for review Pull requests that require a review before continuing label Jan 25, 2025
@fehmer
Copy link
Member

fehmer commented Jan 25, 2025 •
edited
Loading

Hi @fehmer,

Thank you for the feedback! I’ll address the code comment as suggested. Regarding the backend part, I’d be happy to take it up. Could you please share the details or scope of the task?

Same as for the frontend for the files services/weekly-xp-leaderboard.ts and utils/daily-leaderboards.ts

@mohitpaddhariya
Copy link
Author

mohitpaddhariya commented Jan 25, 2025

Hi @fehmer, thank you for the feedback! I’ll address the code comment and work on the backend implementation as well. By the way, could you let me know when this PR will be merged?

@mohitpaddhariya
Copy link
Author

mohitpaddhariya commented Jan 27, 2025

Hi @fehmer, just wanted to follow up regarding the PR. Let me know if there’s anything else needed from my side or an update on the merge timeline. Thanks!

@mohitpaddhariya
Copy link
Author

mohitpaddhariya commented Jan 27, 2025

@Miodec could you tell me what should i need to do next?

@mohitpaddhariya
Copy link
Author

mohitpaddhariya commented Jan 27, 2025 •
edited
Loading

@fehmer @Miodec can you guys reply?

@fehmer
Copy link
Member

fehmer commented Jan 27, 2025

hi @dev-mohit06 , your PR looks good from my side. next mio will review and merge when he has time. You don't need to follow up or rebase unless requested. Thanks for your contribution

@mohitpaddhariya
Copy link
Author

mohitpaddhariya commented Jan 27, 2025

@fehmer, you mentioned the backend file changes. I’ve completed those updates. Should I create the PR now?

@mohitpaddhariya
Copy link
Author

mohitpaddhariya commented Jan 28, 2025

@Miodec Could you please merge this branch? It has already been approved by @fehmer. Thank you!

@Miodec
Copy link
Member

Miodec commented Jan 29, 2025

Ive been sick with some kind of flu lately. Please just be patient, Ill get to all PRs eventually.

@mohitpaddhariya
Copy link
Author

mohitpaddhariya commented Jan 30, 2025

Ive been sick with some kind of flu lately. Please just be patient, Ill get to all PRs eventually.

@Miodec No worries! Take your time and focus on getting better.

@fehmer fehmer mentioned this pull request Feb 3, 2025
Merged
@Miodec
improve function type
6481ae0 
removes unnecessary type casts
@monkeytypegeorge monkeytypegeorge added the packages Changes in local packages label Feb 3, 2025
Miodec
Miodec approved these changes Feb 3, 2025
View reviewed changes
Copy link
Member

@Miodec Miodec left a comment •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks, well done. Sorry it took a while to review.

@Miodec Miodec merged commit 3510ea9 into monkeytypegame:master Feb 3, 2025
15 checks passed
Miodec pushed a commit that referenced this pull request Feb 4, 2025
@fehmer
fix: fix error handling in parseWithSchema (@fehmer) (#6229)
12e1500 
`instanceof ZodError` is not working if the code is packaged as a module
for unknown reason. Found while adding tests for the url-handler in
#6207.
@nadalaba nadalaba mentioned this pull request Mar 17, 2025
Closed
@byseif21 byseif21 mentioned this pull request Apr 22, 2025
Merged
Miodec added a commit that referenced this pull request Apr 24, 2025
@byseif21 @Miodec
refactor(backend): improve redis and json.parse type safety with zod (@…
86383cf 
…byseif21, @Miodec) (#6481)

### Description

refactored backend files to enhance type safety and reliability using
Zod validation and Redis instead of JSON.parse , I tried to avoid the
files that isn't necessary tho so I hope I don't miss any or included
unnecessary ones!! didn't fully test only verified code compilation and
partial tests without Redis!!.


Should Close #5881 
Related to #6207

---------

Co-authored-by: Miodec <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@Miodec Miodec Miodec approved these changes

@fehmer fehmer fehmer left review comments

Assignees

No one assigned

Labels

frontend User interface or web stuff packages Changes in local packages

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

4 participants

@mohitpaddhariya @fehmer @Miodec @monkeytypegeorge